refactor(logstash): drop PKCS#8 filename shim, use native PKCS#8 generation#131
Merged
refactor(logstash): drop PKCS#8 filename shim, use native PKCS#8 generation#131
Conversation
…ration The unencrypted PKCS#8 PEM key now lands directly at `<hostname>.key` across all three cert modes; the `-pkcs8.key` filename is gone. Standalone mode uses `community.crypto.openssl_privatekey` with `format: pkcs8`, which removes the `openssl genrsa` + `openssl pkcs8` shell two-step. The ES_CA flow decrypts straight from the certutil unarchive into the target path instead of copy-then-convert. External mode just uses whatever filename you supplied. Adds `logstash_tls_copy_certs` (default `true`, backward-compatible). Set to `false` in external mode to skip the copy and reference the operator-managed paths directly. This makes certmonger and cert-manager rotation work without an Ansible re-run because the renewal tool rewrites the files and restarts Logstash itself. A soft warning fires if the key is present but looks unreadable by the logstash group; no hard fail, since plenty of setups manage permissions differently. Molecule verify now asserts the key header is `-----BEGIN PRIVATE KEY-----` so a regression to PKCS#1 or encrypted keys fails loudly. Docs updated: the TLS guide's trust-chain diagram, format table, and a new hands-off rotation section, plus the Logstash reference's key-format section rewritten with per-mode key provenance. Closes #126.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes PKCS#8 conversion and standardizes Logstash private key names to Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
roles/logstash/tasks/logstash-security.yml (1)
9-14:⚠️ Potential issue | 🟠 MajorFail fast when hands-off TLS files are missing.
With
logstash_tls_copy_certs: false, this branch only warns about key readability. Missing cert/key/CA paths still flow into the rendered input config, so the play succeeds and Logstash fails on restart. In particular, the computed CA path falls back to{{ logstash_certs_dir }}/ca.crtwhenlogstash_tls_ca_fileis unset, but this branch never creates or validates that file.Possible guardrail
- name: logstash-security | Check external key readability (hands-off mode) when: not (logstash_tls_copy_certs | bool) block: + - name: logstash-security | Stat external certificate in place + ansible.builtin.stat: + path: "{{ logstash_tls_certificate_file }}" + register: _logstash_external_cert_stat + - name: logstash-security | Stat external key in place ansible.builtin.stat: path: "{{ logstash_tls_key_file }}" register: _logstash_external_key_stat + + - name: logstash-security | Determine CA path used by the pipeline + ansible.builtin.set_fact: + _logstash_external_ca_path: "{{ logstash_tls_ca_file | default(logstash_certs_dir ~ '/ca.crt') }}" + + - name: logstash-security | Stat external CA in place + ansible.builtin.stat: + path: "{{ _logstash_external_ca_path }}" + register: _logstash_external_ca_stat + + - name: logstash-security | Validate external TLS files exist in place + ansible.builtin.assert: + that: + - _logstash_external_cert_stat.stat.exists + - _logstash_external_key_stat.stat.exists + - _logstash_external_ca_stat.stat.exists + fail_msg: >- + With logstash_tls_copy_certs: false, the referenced TLS files must + already exist on the managed node.Also applies to: 61-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/logstash/tasks/logstash-security.yml` around lines 9 - 14, The current "logstash-security | Validate external certificate paths are provided" task must fail fast when logstash_tls_copy_certs is false by asserting presence and readability of all required TLS files; update that task (and the similar block at the 61-83 region) to check logstash_tls_certificate_file, logstash_tls_key_file and logstash_tls_ca_file (or the computed fallback {{ logstash_certs_dir }}/ca.crt) are defined and exist/readable (use ansible.builtin.stat and assert on stat.exists/stat.readable) when logstash_tls_copy_certs == false so the play fails early instead of rendering invalid configs that break Logstash on restart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@roles/logstash/tasks/logstash-security.yml`:
- Around line 323-331: The task "logstash-security | Generate server key" using
community.crypto.openssl_privatekey for path "{{ logstash_certs_dir }}/{{
inventory_hostname }}.key" will recreate keys if the existing file is a
different format; update that task to add format_mismatch: convert so the module
converts an existing private key to format: pkcs8 instead of generating a new
key (preserving the key fingerprint and certificate relationship) while keeping
the existing size, type, owner, group and mode settings unchanged.
---
Outside diff comments:
In `@roles/logstash/tasks/logstash-security.yml`:
- Around line 9-14: The current "logstash-security | Validate external
certificate paths are provided" task must fail fast when logstash_tls_copy_certs
is false by asserting presence and readability of all required TLS files; update
that task (and the similar block at the 61-83 region) to check
logstash_tls_certificate_file, logstash_tls_key_file and logstash_tls_ca_file
(or the computed fallback {{ logstash_certs_dir }}/ca.crt) are defined and
exist/readable (use ansible.builtin.stat and assert on
stat.exists/stat.readable) when logstash_tls_copy_certs == false so the play
fails early instead of rendering invalid configs that break Logstash on restart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fac2a2d-ebca-4eaa-80d6-01314f7a1606
📒 Files selected for processing (12)
CHANGELOG.mddocs/guide/tls.mddocs/reference/logstash.mdmolecule/logstash_ssl/converge.ymlmolecule/logstash_ssl/verify.ymlmolecule/logstash_standalone_certs/converge.ymlmolecule/logstash_standalone_certs/verify.ymlmolecule/shared/generate_test_certs.ymlroles/logstash/defaults/main.ymlroles/logstash/tasks/logstash-security.ymlroles/logstash/tasks/main.ymlroles/logstash/templates/10-input.conf.j2
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
roles/logstash/tasks/logstash-security.yml (1)
97-110: Broaden the hands-off warning predicate.The check on Lines 108-110 will warn on valid layouts like
root:logstash 0440or a key owned bylogstashwith0600. That makes the warning noisy even when Logstash can read the file. Consider keying this off owner/group and the relevant read bit instead of a small mode whitelist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/logstash/tasks/logstash-security.yml` around lines 97 - 110, The warning is too strict because it whitelists modes rather than checking whether the logstash user actually has read access; update the when predicate on the "logstash-security | Warn if external key is not readable by logstash group" task to: only run when the file exists and is not readable by the logstash user or group by inspecting _logstash_external_key_stat.stat.pw_name and .gr_name and testing the mode read bits (convert _logstash_external_key_stat.stat.mode to an int with base=8 and check owner-read (0o400) if pw_name == 'logstash' or group-read (0o040) if gr_name == 'logstash'); keep the logstash_tls_key_file and logstash_tls_copy_certs variables unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@roles/logstash/tasks/logstash-security.yml`:
- Around line 273-286: The task "logstash-security | Decrypt key to unencrypted
PKCS#8 PEM" currently uses the ansible.builtin.command invoking "openssl pkcs8
... -in {{ logstash_certs_dir }}/{{ ansible_facts.hostname }}/{{
ansible_facts.hostname }}.key ... -out {{ logstash_certs_dir }}/{{
inventory_hostname }}.key" and is guarded by a creates: "{{ logstash_certs_dir
}}/{{ inventory_hostname }}.key" which prevents re-decryption on cert renewal;
remove the creates: line (the creates guard) from that task so the openssl pkcs8
command always runs and the private key is re-decrypted to match renewed
certificates.
---
Nitpick comments:
In `@roles/logstash/tasks/logstash-security.yml`:
- Around line 97-110: The warning is too strict because it whitelists modes
rather than checking whether the logstash user actually has read access; update
the when predicate on the "logstash-security | Warn if external key is not
readable by logstash group" task to: only run when the file exists and is not
readable by the logstash user or group by inspecting
_logstash_external_key_stat.stat.pw_name and .gr_name and testing the mode read
bits (convert _logstash_external_key_stat.stat.mode to an int with base=8 and
check owner-read (0o400) if pw_name == 'logstash' or group-read (0o040) if
gr_name == 'logstash'); keep the logstash_tls_key_file and
logstash_tls_copy_certs variables unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a6ba692-dd7f-4758-b286-2fd99723603c
📒 Files selected for processing (1)
roles/logstash/tasks/logstash-security.yml
…eadability check Drop the `creates:` guard on the openssl pkcs8 decrypt task. The accompanying server-cert copy is unconditional, so guarding the key decrypt left the renewed cert paired with the previous key and Logstash refused to start until the next manual cleanup. Replace the mode whitelist on the hands-off external-key warning with a check that mirrors what Logstash actually needs: owner/group ownership combined with the corresponding read bit (or a world-read bit). The previous predicate fired on perfectly valid layouts like `root:logstash 0440` and stayed silent on some that wouldn't actually work.
ansible-lint flagged no-changed-when after the creates: guard was removed. The openssl pkcs8 task only runs when the surrounding renewal block fires (_logstash_needs_cert_renewal), and a fresh decrypt always replaces the file, so changed_when: true is the honest signal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The unencrypted PKCS#8 PEM key now lands directly at
<hostname>.keyacross all three cert modes; the-pkcs8.keyfilename is gone. Standalone mode usescommunity.crypto.openssl_privatekeywithformat: pkcs8, which removes theopenssl genrsa+openssl pkcs8shell two-step. The ES_CA flow decrypts straight from the certutil unarchive into the target path instead of copy-then-convert. External mode just uses whatever filename you supplied.Adds
logstash_tls_copy_certs(defaulttrue, backward-compatible). Set tofalsein external mode to skip the copy and reference the operator-managed paths directly. That makes certmonger and cert-manager rotation work without an Ansible re-run because the renewal tool rewrites the files and restarts Logstash itself. A soft warning fires if the key is present but looks unreadable by the logstash group; no hard fail, since plenty of setups manage permissions differently.Molecule verify now asserts the key header is
-----BEGIN PRIVATE KEY-----so a regression to PKCS#1 or encrypted keys fails loudly. Docs updated: the TLS guide's trust-chain diagram, format table, and a new hands-off rotation section, plus the Logstash reference's key-format section rewritten with per-mode key provenance.Closes #126.
Summary by CodeRabbit
New Features
Documentation
Improvements